Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache checked files in fs #8388

Merged
merged 6 commits into from
May 1, 2019

Conversation

Connormiha
Copy link
Contributor

@Connormiha Connormiha commented Apr 28, 2019

Summary

For the same path, the function fs.statSync can call many times. The more test files, the more often it will be called for the same file or directory.
I just added hash table to avoid repeated call for fs.statSync.

Test plan

@codecov-io
Copy link

codecov-io commented Apr 28, 2019

Codecov Report

Merging #8388 into master will increase coverage by 0.02%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8388      +/-   ##
==========================================
+ Coverage   62.34%   62.37%   +0.02%     
==========================================
  Files         266      266              
  Lines       10734    10739       +5     
  Branches     2610     2613       +3     
==========================================
+ Hits         6692     6698       +6     
+ Misses       3460     3459       -1     
  Partials      582      582
Impacted Files Coverage Δ
packages/jest-resolve/src/defaultResolver.ts 68.11% <88.23%> (+4.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff9985c...4bf5f36. Read the comment docs.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. Can you update the changleog?
Btw, do you happen to have any perf measurements on whether it improves stuff?

@jeysal
Copy link
Contributor

jeysal commented Apr 28, 2019

I also think some perf numbers would be great. Caches (esp. those that are never invalidated) can cause problems, so I think we should only introduce this extra complexity for a good reason.

@Connormiha
Copy link
Contributor Author

I created some function performer for testing isFile function.

function perfDecorator(fn) {
  const results = [];
  function printResult() {
    console.log('Perf result start:');
    console.log('Total calls:', results.length, ' ');
    console.log('Min time:', Math.min(...results));
    console.log('Max time:', Math.max(...results));
    console.log(
      'Average time:',
      results.reduce((acc, next) => acc + next, 0) / results.length,
    );
    console.log(
      'Sum time:',
      results.reduce((acc, next) => acc + next, 0),
    );
    console.log('Uniq paths:', typeof checkedPaths === 'Object' ? Object.keys(checkedPaths).length) : '';
    console.log('Perf result end');
  }
  return function(path) {
    let time = process.hrtime()[1];
    const fnResult = fn(path);
    time = process.hrtime()[1] - time;
    results.push(time);
    if (results.length === 4500) {
        printResult();
    }
    return fnResult;
  };
}

isFile = perfDecorator(isFile);

I tested it in my some project.
Total calls isFile: 4500
Uniq paths: 3697
About 20% calsl got from cache.

Before

Perf result start:
Total calls: 4500  
Min time: 7804
Max time: 5951437
Average time: 54111.720888888885
Sum time: 243502744
Perf result end

After

Perf result start:
Total calls: 4500  
Min time: 543
Max time: 5578729
Average time: 37700.06844444444
Sum time: 169650308
Perf result end

@thymikee

@StringEpsilon
Copy link
Contributor

StringEpsilon commented Apr 29, 2019

Not exactly scientific, but I tested this patch against our test suite with some promising results. Thought you might appreciate a real world report. Each run is just npx jest over the full project.

run current patched
1 57.8 s 52.5 s
2 56.1 s 49.0 s
3 55.5 s 53.0 s
4 57.9 s 49.8 s
5 56.9 s 48.8 s
avg 56.8 s 50.6 s

I could not detect any regressions for single-file test suites with the time reported by jest.

@SimenB
Copy link
Member

SimenB commented Apr 29, 2019

Woah, that's great!

Only thing I'm worried about is if the path starts pointing to a different file or it's a file instead of a directory - do we do any cache invalidation here?

@StringEpsilon
Copy link
Contributor

StringEpsilon commented Apr 29, 2019

I'd think that cache invalidation is mostly a problem in watch mode. You could simply bypass the caching completely in watch mode for an easy fix.

Apart from watch mode, the only other sources of file changes during runtime I could think of are:

  1. Newly generated snapshot files
  2. Jest internal cache files
  3. If a user changes something while jest is running.

I tested the first scenario by deleting and re-populating all our snapshots (>200) without any problems. The created files match perfectly according to git. With respect to number 3, jest is already running into problems with weird errors, so I don't think this PR matters in that respect. Though it might make future improvements harder.

With respect to number 2, you know better if jest-resolve is even used in those cases and what the mechanism for cache invalidation should look like.

@SimenB
Copy link
Member

SimenB commented Apr 30, 2019

It's watch mode I was thinking of, yes. Maybe we could just clear the map when starting a new test run?

@jeysal
Copy link
Contributor

jeysal commented Apr 30, 2019

Should the cache perhaps be in Resolver itself instead of the default resolver impl? That would probably make resetting a lot cleaner as well.

@jeysal
Copy link
Contributor

jeysal commented Apr 30, 2019

BTW testing this on a company React app test suite showed about 8% reduction in total time. Pretty good 👍

@Connormiha
Copy link
Contributor Author

It's watch mode I was thinking of, yes. Maybe we could just clear the map when starting a new test run?

I tried run with --watch mode. This patch doesn't effect it because defaultResolver re-required after change files in project. So map refreshed. You can check it just print console.log(checkedPaths.size) inside module. Well, or I do not understand something ((.

const checkedPaths = new Map<string, number>();
console.log('We are here and we have clear Map', checkedPaths.size);

And after run test with --watch, and after try to change your tests.
@SimenB

Copy link
Contributor

@scotthovestadt scotthovestadt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this. I'm going to verify myself that the invalidation is working in watch mode. If we can swap out those values to an enum, we can get this in the release this week!

packages/jest-resolve/src/defaultResolver.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
*/
function isFile(file: Config.Path): boolean {
let result;
enum IPathType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big deal, but FYI-- not using the "prefix with I" convention in the Jest codebase.

@scotthovestadt scotthovestadt merged commit a9c0e6b into jestjs:master May 1, 2019
@ArtemGovorov
Copy link
Contributor

ArtemGovorov commented May 14, 2019

@scotthovestadt @Connormiha The change is breaking Jest watch mode. Here is a simple test:

  • start a simple failing test that imports an inexistent module in watch mode,
  • add the module while in watch mode,
  • the test is still failing:

ji

@Connormiha
Copy link
Contributor Author

@ArtemGovorov Patch arrived with 24.9.0.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants